Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36160 - Redefine append domain names setting #9613

Merged

Conversation

Dyrkon
Copy link
Contributor

@Dyrkon Dyrkon commented Feb 2, 2023

This PR aims to unify the format of host names stored in the database and the way they are displayed. With this change, the name of the host is always going to be stored with the domain name appended.

The setting formerly named append_domain_name_for_hosts is now renamed to display_fqdn_for_hosts because it will only impact how the names are displayed from now. This means dashboards and breadcrumbs are going to display the whole FQDN if you choose to.

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@theforeman-bot
Copy link
Member

Can one of the admins verify this patch?

@ares
Copy link
Member

ares commented Feb 3, 2023

There are more places that now needs to stop using the setting, e.g.

Setting[:append_domain_name_for_hosts] ? @interface.name : @interface.shortname
or the host form, right?

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few other places to update:

  • app/views/hosts/_form.html.erb
  • app/services/name_synchronizer.rb
  • test/unit/name_synchronizer_test.rb
  • test/models/lookup_value_test.rb

and rubocop

app/views/api/v2/hosts/base.json.rabl Outdated Show resolved Hide resolved
app/models/host/base.rb Outdated Show resolved Hide resolved
app/models/host/base.rb Outdated Show resolved Hide resolved
app/models/host/base.rb Show resolved Hide resolved
app/views/api/v2/hosts/base.json.rabl Outdated Show resolved Hide resolved
@ofedoren
Copy link
Member

What about this setting?
ScreenShot-1676301195050

I guess this PR is going to force FQDN to always be stored as a host name, isn't it? If so, does this affect only managed hosts? What about unmanaged or virtual?

Also, do I understand right that from now on the setting should change only the way how to display name of a host? Like in "display short or FQDN" manner with always saving fqdn? I'd also vote to renaming the setting or at least mention that this affect only the view in the description.

Related to the problem: currently I'm kinda mad due to upcoming patches and fixes for UI (#9047 and #9140 and #9201 and #9564 and #9591) to fix almost the same thing again and again. Could we maybe unify the logic and the place where we display hostname? Currently I found few places (https://github.com/theforeman/foreman/blob/develop/app/helpers/hosts_helper.rb#L250 https://github.com/theforeman/foreman/blob/develop/app/models/concerns/orchestration/compute.rb#L47 https://github.com/theforeman/foreman/blob/develop/app/services/name_synchronizer.rb#L28 https://github.com/theforeman/foreman/blob/develop/app/assets/javascripts/host_edit_interfaces.js#L71

return domain_name ? fqdn(host_name, domain_name) : host_name;
) where we deal with host names. Is it me being naive or we really do have mess in it?

@Dyrkon
Copy link
Contributor Author

Dyrkon commented Feb 15, 2023

What about this setting? ScreenShot-1676301195050

I think that this setting focuses on a different area than the one in this PR. I don't think it is related or should be changed.

I guess this PR is going to force FQDN to always be stored as a host name, isn't it? If so, does this affect only managed hosts? What about unmanaged or virtual?

Yes, this PR proposes always storing the FQDN. I don't know what do you mean by

What about unmanaged or virtual?

The change in model is in base.rb, so it will affect unmanaged as well. It shouldn't have an effect on virtual hosts according to @stejskalleos.

Also, do I understand right that from now on the setting should change only the way how to display name of a host? Like in "display short or FQDN" manner with always saving fqdn? I'd also vote to renaming the setting or at least mention that this affect only the view in the description.

Yes, you understand correctly that the setting will only control the view and I already changed the setting description.

Related to the problem: currently I'm kinda mad due to upcoming patches and fixes for UI (#9047 and #9140 and #9201 and #9564 and #9591) to fix almost the same thing again and again. Could we maybe unify the logic and the place where we display hostname? Currently I found few places (https://github.com/theforeman/foreman/blob/develop/app/helpers/hosts_helper.rb#L250 https://github.com/theforeman/foreman/blob/develop/app/models/concerns/orchestration/compute.rb#L47 https://github.com/theforeman/foreman/blob/develop/app/services/name_synchronizer.rb#L28 https://github.com/theforeman/foreman/blob/develop/app/assets/javascripts/host_edit_interfaces.js#L71

return domain_name ? fqdn(host_name, domain_name) : host_name;

) where we deal with host names. Is it me being naive or we really do have mess in it?

Wouldn't this simplify things by having FQDN at all times and control only the way it is displayed? @ofedoren

@Dyrkon Dyrkon changed the title [WIP] Drop append domain names setting #36160 Drop append domain names setting Mar 3, 2023
@Dyrkon
Copy link
Contributor Author

Dyrkon commented Aug 31, 2023

@Lennonka

Copy link

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion for UI Description microcopy.

type: :boolean,
description: N_('Foreman will append domain names when new hosts are provisioned'),
description: N_('Foreman will display domain names appended to hostnames.'),
Copy link

@Lennonka Lennonka Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
description: N_('Foreman will display domain names appended to hostnames.'),
description: N_('Display names of hosts as FQDNs. If disabled, only display names of hosts as hostnames.'),

Not sure about the terminology here. If "shortname" is a thing, then maybe say "shortnames" instead of "hostnames". But to my understanding it's called hostnames and I'd prefer the common industry term.

Edit: Also I'd remove "Foreman" from the description, because that wouldn't work well with downstream branding.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are spot on. I think someone came up with when naming the variable and it caught on.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JS tests are failing:

Error:   81:5  error  ouiaId property is missing in PatternFly component 'Card'   @theforeman/rules/require-ouiaid
Error:   89:9  error  ouiaId property is missing in PatternFly component 'Modal'  @theforeman/rules/require-ouiaid

Can you add the ouiaid field? It's for our automation framework (I think), check with QAs what the value of the fields should be.

@MariaAga
Copy link
Member

MariaAga commented Sep 4, 2023

js Test failures are not related to this pr, I'll open a pr to fix them

@stejskalleos
Copy link
Contributor

ok to test

This PR aims to unify the format of host names stored in the database and the way they are displayed.
With this change, the name of the host is always going to be stored with the domain name appended.
The setting formerly named `append_domain_name_for_hosts` is now renamed to `display_fqdn_for_hosts`
because it will only impact how the names are displayed from now.
This means dashboards and breadcrumbs are going to display the whole FQDN if you choose to.
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍏 LGTM

Works as expected, let's get this in.

@stejskalleos stejskalleos merged commit a406cf2 into theforeman:develop Oct 9, 2023
13 checks passed
@stejskalleos
Copy link
Contributor

Thanks @Dyrkon and others!

@Dyrkon
Copy link
Contributor Author

Dyrkon commented Oct 9, 2023

Big thanks for everyone who commented and helped with the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants